Fix JavaList.Remove() always-false guard condition (&& -> ||)#11237
Merged
simonrozsival merged 2 commits intomainfrom Apr 30, 2026
Merged
Fix JavaList.Remove() always-false guard condition (&& -> ||)#11237simonrozsival merged 2 commits intomainfrom
&& -> ||)#11237simonrozsival merged 2 commits intomainfrom
Conversation
The guard condition if (i < 0 && i >= Count) was always false because a value cannot simultaneously be negative and >= Count (which is non-negative). This caused Remove() to call RemoveAt(-1) when an item wasn't found, throwing an exception instead of silently no-op-ing as IList.Remove requires. Fix: change && to || so the guard correctly returns early when IndexOf returns -1 (item not found) or when a race condition shrinks the list between IndexOf and RemoveAt. Add a test to verify Remove() with a non-existent item does not throw. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes JavaList.Remove(object?) to correctly no-op (per IList.Remove contract) when the item is not found, avoiding an out-of-range removal call.
Changes:
- Corrected an always-false guard condition in
JavaList.Remove(&&→||) to preventRemoveAt(-1). - Added an NUnit regression test verifying that removing a non-existent item does not throw and does not change the list count.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/Mono.Android/Android.Runtime/JavaList.cs |
Fixes the guard in Remove(object?) so out-of-range indexes are safely ignored. |
tests/Mono.Android-Tests/Mono.Android-Tests/Java.Interop/JavaListTest.cs |
Adds regression coverage to ensure Remove is a no-op when the item is absent. |
The original PR only fixed JavaList.Remove(object?), but the same
bug exists in JavaList.Remove(Java.Lang.Object?) and
JavaList<T>.Remove(T). The test calls list.Remove("bar") which
resolves to the Java.Lang.Object? overload via the implicit
string->Java.Lang.Object conversion operator.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jonathanpeppers
added
the
ready-to-review
jonathanpeppers
changed the title
&& -> ||)
simonrozsival
approved these changes
Apr 30, 2026
Member
|
CI failure is unrelated |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
JavaList.Remove(object?)had an always-false guard condition at line 395:A value cannot simultaneously be
< 0and>= Count(Count is always non-negative), so this guard could never fire. The practical consequence: whenIndexOfreturns-1(item not found), the dead guard passes andRemoveAt(-1)is called, throwing an exception instead of silently no-op-ing as theIList.Removecontract requires.Fix
Change
&&to||:This correctly returns early when:
IndexOfreturns-1(item not in list) — thei < 0branchIndexOfandRemoveAt— thei >= CountbranchChanges
src/Mono.Android/Android.Runtime/JavaList.cs: one-character fix (&&→||)tests/Mono.Android-Tests/Mono.Android-Tests/Java.Interop/JavaListTest.cs: addedRemoveNonExistentItemDoesNotThrowtest — adds"foo", callsRemove("bar"), asserts no exception is thrown and count remains 1